Skip to content

chore: make the maintainer the only approval that satisfies the review gate - #1464

Merged
vivek7405 merged 1 commit into
mainfrom
chore/codeowners-review-gate
Sep 4, 2026
Merged

chore: make the maintainer the only approval that satisfies the review gate#1464
vivek7405 merged 1 commit into
mainfrom
chore/codeowners-review-gate

Conversation

@vivek7405

@vivek7405 vivek7405 commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Closes #1463

Summary

main already required one approving review, but nothing said whose. Now that
the repo takes outside contributions, two drive-by contributors can approve each
other's PRs, and with CI green that is enough to merge. This makes the maintainer
the only approval that satisfies the gate.

Two halves, and they only work as a pair:

  • .github/CODEOWNERS names @vivek7405 for every path.
  • scripts/protect-main.sh flips require_code_owner_reviews to true.

require_code_owner_reviews with no CODEOWNERS file matches nothing and is a
silent no-op, which is why both land in one commit. enforce_admins stays
false, unchanged: GitHub will not let me approve my own PR, so without the
admin bypass every solo PR would be unmergeable.

Re-running the script also repairs a drift I noticed while reading the live rule.
Protection enforces five required contexts, but ci.yml has six jobs and the
script has listed all six for a while, so In-repo app tests (website + blog)
was never actually required. Applying the script fixes that as a side effect.

Applying it

The script is the source of truth but nothing runs it automatically, so the rule
does not change until someone with repo admin runs it:

bash scripts/protect-main.sh

I will run it after this merges, and confirm with
gh api repos/webjsdev/webjs/branches/main/protection.

Definition of done

  • Tests. N/A because nothing here is framework code. The change is a GitHub
    config file plus the shell script that pushes branch protection through the
    API; there is no unit, browser, e2e, smoke, or Bun surface to assert against.
    It is verified by reading back the live protection object after the script
    runs, which is in the Applying it section above.
  • Markdown. Updated .claude/skills/webjs-start-work/SKILL.md, whose merge
    section described the gate as five contexts and did not mention who the
    approval has to come from. Both corrected. No other markdown describes branch
    protection (checked with a repo-wide grep for protect-main, branch protection, approving review, CODEOWNERS).
  • Docs site. N/A because the review gate is repo governance, not a webjs API
    a user reads about.
  • Scaffold, MCP, editor plugins, marketing copy. N/A because none of them
    project repo governance.
  • Dogfood boot check. N/A because no framework code, dist output, importmap,
    or served wire changed. Nothing the blog or website serves is affected.
  • Version bumps. N/A because no published package changed.

https://claude.ai/code/session_01MYGjB4NnsBZaDktR2E4y2Q

@vivek7405
chore: make the maintainer the only approval that satisfies the gate
a777294 
main already required one approving review, but nothing said whose. In a
public repo two drive-by contributors can approve each other, and with the
CI checks green that is enough to merge. A CODEOWNERS file naming me for
every path plus require_code_owner_reviews closes that: the one required
approval now has to be mine.

The two halves only work together. require_code_owner_reviews with no
CODEOWNERS matches nothing and is a silent no-op, which is why the setting
and the file land in the same commit.

Re-running the script also repairs a drift I found while checking the live
rule: it enforces five contexts, but ci.yml has six jobs and the script has
listed all six for a while. "In-repo app tests (website + blog)" was never
required. The skill doc said five as well, so it is corrected to six.

Claude-Session: https://claude.ai/code/session_01MYGjB4NnsBZaDktR2E4y2Q
@vivek7405 vivek7405 self-assigned this Sep 4, 2026
@vivek7405

vivek7405 commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author

Decision: a CODEOWNERS gate rather than a second required approval, and why enforce_admins stays off

Two alternatives I looked at and rejected before settling on this shape.

Raising required_approving_review_count to 2. It reads like the stronger rule
but it is the wrong one for this repo. GitHub does not count the author, so on a
solo PR it demands two outside approvals that do not exist, and it still does not
say WHO the approvals come from, which is the actual gap. Two contributors
approving each other satisfies a count of 2 exactly as easily as a count of 1.
The gap is identity, not quantity, so the fix belongs on identity.

Moving to a repository ruleset instead of classic branch protection. Rulesets are
where GitHub is heading and they can layer, but scripts/protect-main.sh already
expresses the whole rule as one idempotent PUT that anyone can read in twenty
lines. Porting it would be a rewrite that changes nothing observable about the
gate. Not worth it today. If the rule ever needs to differ per branch pattern,
that is the moment to move.

The tradeoff I am taking with eyes open: enforce_admins stays false, so I can
still merge my own PRs through the admin bypass. That means this rule constrains
contributors and not me. That is deliberate and it is the only workable setting
while I am the sole maintainer, because GitHub refuses to let a PR author approve
their own PR, so with enforce_admins: true every PR I open would be permanently
unmergeable. The existing note at the top of the script already says this; the
CODEOWNERS half does not change the calculus. When a second reviewer exists,
flipping enforce_admins to true is the follow-on, and at that point the
CODEOWNERS file is what makes it mean something.

Follow-up: nothing applies this script automatically

Worth writing down because it has bitten once already. The script is the source
of truth for the rule, but it is a script, not a workflow. Merging this PR does
not change branch protection by itself. I run it by hand after the merge. That is
also how I found the drift the body mentions: the live rule had five required
contexts while the script has listed six for a while, which means the script had
not been re-run since In-repo app tests (website + blog) was added to ci.yml.
Any future edit to the required-contexts list has the same failure mode.

@vivek7405
vivek7405 marked this pull request as ready for review September 4, 2026 07:47
@vivek7405
vivek7405 merged commit 0394619 into main Sep 4, 2026
10 checks passed
@vivek7405
vivek7405 deleted the chore/codeowners-review-gate branch September 4, 2026 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@vivek7405 vivek7405

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Make the maintainer the only approval that can satisfy the review gate

1 participant

@vivek7405